Skip to content

Improve UPnP configuration flow#34737

Merged
balloob merged 18 commits intohome-assistant:devfrom
StevenLooman:upnp_config_flow
May 3, 2020
Merged

Improve UPnP configuration flow#34737
balloob merged 18 commits intohome-assistant:devfrom
StevenLooman:upnp_config_flow

Conversation

@StevenLooman
Copy link
Copy Markdown
Contributor

@StevenLooman StevenLooman commented Apr 26, 2020 •
edited
Loading

Breaking change

Proposed change

Improvements to the configuration flow by using a custom flow:

  • Multiple devices are now supported, instead of a single device.
  • Configuration flows also have a unique ID now, making them ignorable.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@StevenLooman StevenLooman mentioned this pull request Apr 26, 2020
Merged
8 tasks
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/__init__.py Outdated
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/__init__.py Outdated
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py Outdated
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py Outdated
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py Outdated
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py Outdated
balloob
balloob reviewed Apr 27, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/device.py
@property
def unique_id(self) -> str:
"""Get the unique id."""
return f"{self.udn}::{self.device_type}"
Copy link
Copy Markdown
Member

@balloob balloob Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this include ST?

Copy link
Copy Markdown
Contributor Author

@StevenLooman StevenLooman Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing. The SSDP protocol provides a USN (UDN and ST combined.) E.g., a search-response is something like (converted to JSON, as the CLI for async_upnp_client presents it like so, keys prefixes with _ added by library):

{"Cache-Control": "max-age=1900", "Location": "http://192.168.178.1:80/RootDevice.xml", "Server": "UPnP/1.0 UPnP/1.0 UPnP-Device-Host/1.0", "ST": "urn:schemas-upnp-org:device:InternetGatewayDevice:1", "USN": "uuid:upnp-InternetGatewayDevice-1_0-889ffacb9043::urn:schemas-upnp-org:device:InternetGatewayDevice:1", "EXT": "", "_timestamp": "2020-04-27 21:33:01.811349", "_address": "192.168.178.1:1900", "_udn": "uuid:upnp-InternetGatewayDevice-1_0-889ffacb9043", "_source": "search"}

The UPnP XML description provides a UDN and a deviceType (together these form the USN from the SSDP protocol), e.g.:

<?xml version="1.0" encoding="utf-8"?>
<root xmlns="urn:schemas-upnp-org:device-1-0">
  ...
  <device>
    <deviceType>urn:schemas-upnp-org:device:InternetGatewayDevice:1</deviceType>
    <UDN>uuid:upnp-InternetGatewayDevice-1_0-889ffacb9043</UDN>
    ...
  </device>
</root>

Some criticism from my side: The SSDP component in hass returns the XML (in dict form), but entirely discards the search-information. Can be easily worked around, but requires a bit of additional work.

Copy link
Copy Markdown
Member

@balloob balloob Apr 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Feel free to update the SSDP integration 👍 (although we use netdisco for ssdp scan, which has been deprecated…)

Copy link
Copy Markdown
Contributor Author

@StevenLooman StevenLooman May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find the required information in the current implementation. It probably requires a change to netdisco.

What do you propose to do with SSDP, rewrite it to drop netdisco? Should it use its own search implementation or use a library for this? (async_upnp_client provides this, which is what the upnp component uses.) Also, devices also send out advertisements via SSDP, which home assistant could use to passively listen for, instead of 'actively' searching for devices.

Copy link
Copy Markdown
Member

@balloob balloob May 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, async_upnp_client and passive event listening sound good 👍 netdisco has been deprecated and should be dropped.

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 27, 2020

So it seems like this fixes an issue in the beta that upnp config flows cannot be ignored. However this is quite a change and the release is already on Wednesday.

We should remove ssdp discovery for the upnp integration in 0.109 so that we can add this feature in 0.110 without having to do major changes last-minute which won't be able to be tested properly.

@balloob balloob mentioned this pull request Apr 27, 2020
Merged
20 tasks
@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented Apr 27, 2020

We should remove ssdp discovery for the upnp integration in 0.109 so that we can add this feature in 0.110 without having to do major changes last-minute which won't be able to be tested properly.

Agreed.

@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented Apr 27, 2020 •
edited
Loading

Also, I would like to simplify this component by the following things:

  • No longer provide port forwards. Although handy, it does provide a potential security risk. If one wishes to make the home assistant instance accessible from the internet, wouldn't it be better to manually configure their firewall/router? This feature is already only available via configuration.yaml and not via the configuration flows, thus already preventing one from accidentally enabling this.
  • Always enable sensors. If the previous point is carried through, upnp has no value at all. Also, it simplifies configuration and code.

This is, of course, a regression wrt features it provides!

@balloob @dgomes Is this acceptable? If so, I'll make a separate pull request for this.

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 27, 2020

I agree.

@StevenLooman StevenLooman changed the title Improve UPnP configuration flow WIP: Improve UPnP configuration flow Apr 27, 2020
@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented Apr 27, 2020

Please don't merge yet, I think hass can lock up when upnp is added via configuration.yaml.

@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented Apr 30, 2020

Alright, @balloob, care for another review?

balloob
balloob reviewed Apr 30, 2020
View reviewed changes
Comment thread homeassistant/components/discovery/__init__.py Outdated
balloob
balloob reviewed Apr 30, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/translations/en.json Outdated
balloob
balloob reviewed Apr 30, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/translations/en.json Outdated
balloob
balloob reviewed Apr 30, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/config_flow.py Outdated
balloob
balloob reviewed May 2, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/strings.json Outdated
balloob
balloob reviewed May 2, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/strings.json Outdated
balloob
balloob reviewed May 2, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/strings.json Outdated
balloob
balloob reviewed May 2, 2020
View reviewed changes
Comment thread tests/components/upnp/test_init.py
@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented May 2, 2020

I cannot reproduce the failures from the unit tests. Also, I think the failures are incorrect. One of the tests fails with:

Traceback (most recent call last):
  File "/__w/1/s/homeassistant/data_entry_flow.py", line 123, in async_init
    result = await self._async_handle_step(
  File "/__w/1/s/homeassistant/data_entry_flow.py", line 196, in _async_handle_step
    result: Dict = await getattr(flow, method)(user_input)
  File "/__w/1/s/homeassistant/components/upnp/config_flow.py", line 121, in async_step_import
    discovery = self._discoveries[0]
TypeError: '_asyncio.Future' object is not subscriptable

The part before the error does await the future (config_flow.py:114):

self._discoveries = await Device.async_discover(self.hass)

The test also provides a mocked co-rountine for this.

Python37 runs without problems. Locally, running python 3.8.0 runs without any problems. CI seems to be using python 3.8.2, which fails.

Unsure what to do here.

@balloob
Copy link
Copy Markdown
Member

balloob commented May 3, 2020

It's because we use asynctests on py37 and unittest.testcase on py38 which is compatible with asyncio. They are very similar but there are some small differences, patch.object is one of those. Fixed the tests for you 👍

@balloob balloob merged commit 6afb42b into home-assistant:dev May 3, 2020
@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented May 3, 2020

Thank you for fixing and merging, @balloob!

@StevenLooman
Copy link
Copy Markdown
Contributor Author

StevenLooman commented May 3, 2020

home-assistant/home-assistant.io#13180 (docs) can be merged as wel, after a review.

@StevenLooman StevenLooman deleted the upnp_config_flow branch May 3, 2020 19:12
balloob
balloob reviewed May 3, 2020
View reviewed changes
Comment thread homeassistant/components/upnp/strings.json
"init": {
},
"ssdp_confirm": {
"description": "Do you want to set up this UPnP/IGD device?"
Copy link
Copy Markdown
Member

@balloob balloob May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that I was thinking last night is that most people don't know what UPnP/IGD is, or how it comes that Home Assistant discovers one. I think that we might want to extend this description to explain what it is/what it can be used for. What do you think ?

Copy link
Copy Markdown
Contributor Author

@StevenLooman StevenLooman May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Perhaps something like:

A router has been discovered in your network. You can monitor the traffic statistics for this device. Do you want to set up this integration?

Copy link
Copy Markdown
Contributor Author

@StevenLooman StevenLooman May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware though: Some other devices also provide information via UPnP/IGD profile, such as access points.

@StevenLooman StevenLooman mentioned this pull request May 4, 2020
Merged
20 tasks
@lock lock Bot locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@balloob balloob balloob approved these changes

Assignees

No one assigned

Labels

by-code-owner cla-signed core has-tests integration: discovery integration: upnp new-feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unable to prevent discovery of UPnP/IGD in 0.109

3 participants

@StevenLooman @balloob @homeassistant